cli/context/store: cap tls file size on zip import#6934
Conversation
`io.ReadAll(f)` on the decompressed tls/* entries was unbounded, so a zip whose compressed archive is within the 10 MiB outer cap could still decompress to multi-gigabyte TLS files and OOM the CLI. The meta.json branch right above already wraps its reader in limitedReader, this just mirrors it for the tls branch. Also fast-fails on the advertised UncompressedSize64 before calling zf.Open, so a well-formed zip bomb is rejected without any decompression at all. limitedReader still guards the stream in case the header lies. Fixes docker#6917 Signed-off-by: texasich <texasich@users.noreply.github.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Pulls the per-entry TLS decoding out of importZip so the outer loop stays under gocyclo's complexity limit. Pure refactor, no behavior change. Signed-off-by: texasich <texasich@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds zip-bomb protection for TLS materials when importing contexts from ZIP archives, ensuring oversized TLS entries are rejected without unbounded memory usage.
Changes:
- Introduces
importTLSEntryto enforce per-file size limits fortls/ZIP entries. - Adds a regression test to ensure oversized TLS entries are rejected with an appropriate error.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cli/context/store/store.go | Adds TLS ZIP-entry import helper with size checks and limited reading to prevent zip-bomb allocations. |
| cli/context/store/store_test.go | Adds a test covering rejection of oversized TLS entries during ZIP import. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Defense in depth in case the zip header is spoofed. | ||
| data, err := io.ReadAll(&limitedReader{R: f, N: maxAllowedFileSizeToImport}) | ||
| if err != nil { | ||
| return err | ||
| } |
| // Write well over the per-file cap; zeros compress to a tiny archive | ||
| // so the outer archive-size cap is not hit first. | ||
| oversized := make([]byte, 2*maxAllowedFileSizeToImport) | ||
| _, err = tf.Write(oversized) | ||
| assert.NilError(t, err) |
|
thanks for the review notes. i’ll address both points explicitly:
i’ll post an update on this thread with the exact patch/tests once done. |
|
Gentle nudge on this one — been open a while. Happy to rebase if there are conflicts, or make any changes needed. Let me know if anything's blocking review. 🙏 |
What I did
Wrap the
io.ReadAllcall fortls/*entries inimportZipwith the existinglimitedReader, and add a fast-fail check againstzf.UncompressedSize64before opening the entry. Fixes #6917.How I did it
The
meta.jsonbranch a few lines above already uses&limitedReader{R: f, N: maxAllowedFileSizeToImport}; thetls/branch was still callingio.ReadAll(f)directly, which is unbounded after flate has decompressed. A compressed archive that fits under the outer 10 MiB cap can trivially decompress to multi-GB TLS entries and take the CLI out with an OOM.Primary guard is the header check: if
UncompressedSize64exceeds the cap we bail out before callingzf.Open(), so no decompression happens at all for a well-formed zip bomb. ThelimitedReaderwrapper stays as defense-in-depth for the case where the header lies about the declared size.How to verify it
Added
TestImportZipTLSTooLargeincli/context/store/store_test.go: it builds an in-memory zip whosetls/docker/ca.pementry is2 * maxAllowedFileSizeToImportzero bytes (compresses to ~10 KiB, so the outer archive limit is not what's catching it), and asserts thatImportrejects it with the expected error. Also rango vetandgo buildon linux and windows.Local
go test ./cli/context/store/passes aside from an unrelated pre-existingTestImportZipTempDir cleanup flake on Windows (open handle ontest.zip), which reproduces on master without these changes.